Do not leak active head data in statement distribution #3519

koute · 2021-07-26T08:26:57Z

The main problem here was that this code essentially did an early return through the ? operator on the first error it encountered, effectively ignoring everything else in activated, and also left deactivated completely untouched which resulted in permanently leaking items which were added to active_heads in the past.

I'm not entirely sure if this is the way we're supposed to handle those errors; previously the handle_subsystem_message returned an Err when get_session_index failed; now I've made it to just continue. It's not obvious to me what the right answer is here: should the whole function return an Err if it fails for only one element of activated? I'd appreciate it if someone more experienced with this code could chime in.

ordian · 2021-07-26T09:22:12Z

node/network/statement-distribution/src/lib.rs

+				for deactivated in deactivated {
+					if active_heads.remove(&deactivated).is_some() {
+						tracing::trace!(
+							target: LOG_TARGET,
+							hash = ?deactivated,
+							"Deactivating leaf",
+						);
+					}


I think we can simplify the fix by moving this section above the activated loop. This is equivalent, because we never activate and deactivate a leaf at the same time.

We can revert the rest of the changes, which makes your question obsolete.

We can revert the rest of the changes, which makes your question obsolete.

Actually, no, let's collect all errors to a vec instead.

I'm not entirely sure if this is the way we're supposed to handle those errors;

The logic of handling errors is on the caller side depending on whether it's Fatal or not:

polkadot/node/network/statement-distribution/src/lib.rs

Lines 1632 to 1638 in 5dac532

match result {

Ok(true) => break,

Ok(false) => {}

Err(Error(Fault::Fatal(f))) => return Err(f),

Err(Error(Fault::Err(error))) =>

tracing::debug!(target: LOG_TARGET, ?error)

}

We can revert the rest of the changes, which makes your question obsolete.

Well, not necessarily, since there's still the problem of the rest of elements of activated essentially being ignored if only one's missing. The memory leak here was definitely not intended, and it was a direct result of the early return, so it would make sense that the other elements of activated being ignored were also a mistake.

Actually, no, let's collect all errors to a vec instead.

Sounds good to me.

Well, not necessarily, since there's still the problem of the rest of elements of activated essentially being ignored if only one's missing.

I just realized that activated actually only consists of t most 1 head except for the startup. So the simplest fix would be just moving deactivation handling up.

Well, not necessarily, since there's still the problem of the rest of elements of activated essentially being ignored if only one's missing.

I just realized that activated actually only consists of t most 1 head except for the startup. So the simplest fix would be just moving deactivation handling up.

So just for my own reference - are you saying that it's fine to ignore rest of the elements here, or that there won't ever be any extra elements?

(If it's the latter then it'd be nice to add a debug_assert! here.)

Currently we create active leaves only on block import (1 activated head):

polkadot/node/overseer/src/lib.rs

Lines 727 to 758 in 5dac532

async fn block_imported(&mut self, block: BlockInfo) -> SubsystemResult<()> {

match self.active_leaves.entry(block.hash) {

hash_map::Entry::Vacant(entry) => entry.insert(block.number),

hash_map::Entry::Occupied(entry) => {

debug_assert_eq!(*entry.get(), block.number);

return Ok(());

}

};

let mut update = match self.on_head_activated(&block.hash, Some(block.parent_hash)) {

Some((span, status)) => ActiveLeavesUpdate::start_work(ActivatedLeaf {

hash: block.hash,

number: block.number,

status,

span

}),

None => ActiveLeavesUpdate::default(),

};

if let Some(number) = self.active_leaves.remove(&block.parent_hash) {

debug_assert_eq!(block.number.saturating_sub(1), number);

update.deactivated.push(block.parent_hash);

self.on_head_deactivated(&block.parent_hash);

}

self.clean_up_external_listeners();

if !update.is_empty() {

self.broadcast_signal(OverseerSignal::ActiveLeaves(update)).await?;

}

Ok(())

}

and on startup only once

polkadot/node/service/src/lib.rs

Lines 678 to 680 in c0387ba

let active_leaves = futures::executor::block_on(

active_leaves(&select_chain, &*client)

)?;

(If it's the latter then it'd be nice to add a debug_assert! here.)

We can change the signature to of activated instead to be a single entry. (But that's out of scope of the PR)

So just for my own reference - are you saying that it's fine to ignore rest of the elements here, or that there won't ever be any extra elements?

It's fine to ignore extra elements on startup for statement-distribution and there won't ever be any extra elements after that.

Okay, I've moved the deallocation to the top.

I'm still not exactly comfortable with leaving the loop below as-is; if there's going to be only one element then that definitely should be enforced by the type system, but as you've said that is out of the scope of this PR.

Thanks, I'll handle that as part of #2812.

ordian

Thank you for identifying the issue and bearing with me!

* master: enable disputes (#3478) Do not leak active head data in statement distribution (#3519) Fix TransactAsset Implementation (#3345)

koute requested review from eskimor and ordian July 26, 2021 08:26

ordian reviewed Jul 26, 2021

View reviewed changes

ordian mentioned this pull request Jul 26, 2021

subsystems: use ActiveLeavesUpdate instead of OurViewChange #2812

Open

Do not leak active head data in statement distribution

79ac26b

koute force-pushed the master branch from 8449d2c to 79ac26b Compare July 26, 2021 10:54

ordian approved these changes Jul 26, 2021

View reviewed changes

ordian merged commit 6dfdeab into paritytech:master Jul 26, 2021

ordian added a commit that referenced this pull request Jul 26, 2021

Merge branch 'master' into bernhard-integrate-dispute-finality

88fd180

* master: enable disputes (#3478) Do not leak active head data in statement distribution (#3519) Fix TransactAsset Implementation (#3345)

ordian mentioned this pull request Jul 27, 2021

change ActiveLeaves to contain at most one activated #3525

Merged

eskimor mentioned this pull request Aug 12, 2021

collect better memory stats #3612

Merged

github-actions bot mentioned this pull request Aug 24, 2021

Update substrate/polkadot/cumulus from v0.9.8 to v0.9.9 moonbeam-foundation/moonbeam#729

Closed

koute mentioned this pull request Sep 29, 2022

Member Request polkadot-fellows/seeding#27

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not leak active head data in statement distribution #3519

Do not leak active head data in statement distribution #3519

koute commented Jul 26, 2021

ordian Jul 26, 2021

ordian Jul 26, 2021

koute Jul 26, 2021

koute Jul 26, 2021

ordian Jul 26, 2021

koute Jul 26, 2021

ordian Jul 26, 2021 •

edited

Loading

ordian Jul 26, 2021

koute Jul 26, 2021

ordian Jul 26, 2021

ordian left a comment

	match result {
	Ok(true) => break,
	Ok(false) => {}
	Err(Error(Fault::Fatal(f))) => return Err(f),
	Err(Error(Fault::Err(error))) =>
	tracing::debug!(target: LOG_TARGET, ?error)
	}

	async fn block_imported(&mut self, block: BlockInfo) -> SubsystemResult<()> {
	match self.active_leaves.entry(block.hash) {
	hash_map::Entry::Vacant(entry) => entry.insert(block.number),
	hash_map::Entry::Occupied(entry) => {
	debug_assert_eq!(*entry.get(), block.number);
	return Ok(());
	}
	};

	let mut update = match self.on_head_activated(&block.hash, Some(block.parent_hash)) {
	Some((span, status)) => ActiveLeavesUpdate::start_work(ActivatedLeaf {
	hash: block.hash,
	number: block.number,
	status,
	span
	}),
	None => ActiveLeavesUpdate::default(),
	};

	if let Some(number) = self.active_leaves.remove(&block.parent_hash) {
	debug_assert_eq!(block.number.saturating_sub(1), number);
	update.deactivated.push(block.parent_hash);
	self.on_head_deactivated(&block.parent_hash);
	}

	self.clean_up_external_listeners();

	if !update.is_empty() {
	self.broadcast_signal(OverseerSignal::ActiveLeaves(update)).await?;
	}
	Ok(())
	}

	let active_leaves = futures::executor::block_on(
	active_leaves(&select_chain, &*client)
	)?;

Do not leak active head data in statement distribution #3519

Do not leak active head data in statement distribution #3519

Conversation

koute commented Jul 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordian Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordian left a comment

Choose a reason for hiding this comment

ordian Jul 26, 2021 •

edited

Loading